Skip to content

chore(query): Tuning sort spill memory usage #18520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 13, 2025

Conversation

forsaken628
Copy link
Collaborator

@forsaken628 forsaken628 commented Aug 12, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

  • Adjusting the trigger for the sort spill collect step
  • Adjusting the merge parameters for the sort spill restore step
  • Changing the default value for sort_spilling_batch_bytes
  • Fixing span loss by upgrading fastrace

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Aug 12, 2025
@forsaken628
Copy link
Collaborator Author

forsaken628 commented Aug 12, 2025

Benchmark:

Settings

max_query_memory_usage 2147483648
max_threads 4
query_out_of_memory_behavior spilling
sort_spilling_batch_bytes  1048576

DataSet: tpch sf100

SQL

select * from orders order by o_totalprice desc ignore_result;

Analysis:
In this test, there are only two operators that use a lot of memory, one is sort and the other is scan.
The sort collect step eats up all the available memory within the limit, while scan has no memory limit logic, so if scan allocates a lot of memory at this time, for example, if there are many columns and the columns are relatively large, the limit will be exceeded.

Currently, the sort restore step does not read the global memory usage, but instead statically determines the available memory by referring to the memory usage of the collect step, which is an inelastic strategy. After the restore step, since there is no more scanning, we can't see any problem.

Bug fixed:
If the block coming from upstream is large, it is possible to directly spill it without sorting, which will cause huge deviation in memory estimation of the restore step. This bug can be made more obvious by making sort_spilling_batch_bytes smaller.
The current tuning should ensure that only the first spill in the collect step exceeds the limit.

It is recommended to use a larger sort_spilling_batch_bytes, because the smaller the sort_spilling_batch_bytes, the larger the num_merge, the more fragmented the blocks, and the larger the num_merge, the more destabilizing the elements will be. (More testing is needed on what the destabilizing elements are)

@forsaken628 forsaken628 marked this pull request as ready for review August 12, 2025 07:48
@BohuTANG
Copy link
Member

BohuTANG commented Aug 13, 2025

Two issues with this PR:

  1. conflicts Cargo.lock
  2. test_logs always failed(🤔 )
  3. explain CI failed: https://github.com/databendlabs/databend/actions/runs/16924936615/job/47961073098?pr=18520

@BohuTANG BohuTANG added the ci-cloud Build docker image for cloud test label Aug 13, 2025
Copy link
Contributor

Docker Image for PR

  • tag: pr-18520-fdc90d3-1755051739

note: this image tag is only available for internal use.

@forsaken628 forsaken628 added ci-cloud Build docker image for cloud test and removed ci-cloud Build docker image for cloud test labels Aug 13, 2025
Copy link
Contributor

Docker Image for PR

  • tag: pr-18520-5b6746a-1755083362

note: this image tag is only available for internal use.

@BohuTANG BohuTANG merged commit b6c97a4 into databendlabs:main Aug 13, 2025
101 of 102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-chore this PR only has small changes that no need to record, like coding styles.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants